Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.60%. Comparing base (1621962) to head (3a84ca8).
Report is 6 commits behind head on main.

@@            Coverage Diff             @@
##             main     #146      +/-   ##
==========================================
+ Coverage   98.59%   98.60%   +0.01%     
==========================================
  Files          15       15              
  Lines        1068     1076       +8     
==========================================
+ Hits         1053     1061       +8     
  Misses         15       15              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Thanks @steven-murray, this looks like a great addition.

Summarizing to check I understand the changes:

• The UVWs are computed using the uvutils.phasing.calc_uvw function from pyuvdata. This of course already has support for non-zenith phase centers.
• The main change is in _utils.py, to add a phase_center_dec argument to phase_past_zenith allow off-zenith phasing.
• The phase_past_zenith() is called by Observatory.projected_baselines(), so this needs a phase_center_dec argument added too.
• Similarly phase_center_dec is added to Observation
• When UVW coordinates are calculated, they will be calculated using the RA/DEC of the phase center, which no longer needs to be zenith.

The new code does indeed change UV bins:

I am a little suspicious though, as sensitivity.calculate_significance() now seems to be suggesting better sensitivity away from zenith (due to effective integration times higher in the above image?), which is counter to what I'd expect.

(thinking more about it: I didn't change the beam, and SEFD will increase for off-zenith pointings with an aperture array. I guess those effects are not accounted for in my simple example?)

Update: looks like increasing the beam width will decrease the sensitivity, accounting for decreased projected area. I used a Gaussian beam for my test above, and didn't change its width between the two simulations, hence the odd outcome.

@telegraphic thanks for the review!

So, yes your outline is correct. I have not touched the beam/SEFD in this PR -- though perhaps it's necessary to obtain reasonable results. Currently we don't have the infrastructure to be able to have a beam whose projected area changes over the course of an observation, however we could at least change the area to be consistent with the projected area when the phase center is as close to the zenith as possible?

Yeah, we've kind of always neglected the primary beam response when considering tracked observations. The argument was always that you've either got a steerable dish (and hence the SEFD can stay constant as you follow the field) or you've got an aperture array where you're beamforming (in which case there is an increase of the SEFD off-zenith, but it's small since it's set by the primary beam of a single dipole, not the station/tile, and those are very wide). Realistically you would want to account for it, but I think it's a pretty big change to the inner workings of 21cmSense to make it work.

@telegraphic are you happy for this PR to be merged? We could attempt to implement a changing beam area as a function of pointing, but I'm not sure how much it will affect results in realistic cases (where you're not pointing at the horizon...), and it would be a reasonably involved bit of work.

Hi @steven-murray, yes happy for this to be merged. As @jpober mentioned, for a steerable dish the SEFD would not change, so it seems reasonable to leave to the user to implement any change in beam characteristics by subclassing PrimaryBeam.

FYI, I have used this branch for the SKA-Low EoR sensitivity calculator prototype @ https://github.com/ska-sci-ops/ska-ost-eor-senscalc

@jpober we're missing your review. If you're also happy with the changes, we can merge it after your approval. Thanks.

Sorry, didn't realize I was blocking it. I'll try to have a look tomorrow!